Skip to content

fix(tree): errors following a full review of the component - #1946

Open
lskramarov wants to merge 9 commits into
mainfrom
review/tree
Open

fix(tree): errors following a full review of the component#1946
lskramarov wants to merge 9 commits into
mainfrom
review/tree

Conversation

@lskramarov

@lskramarov lskramarov commented Aug 26, 2026

Copy link
Copy Markdown
Contributor

Summary

The tree is a hard fork of @angular/cdk/tree that re-implements the whole CDK tree concept set, and
it carries the debt of an old fork: a fragile global static node handoff, an 811-line god component,
pervasive any, and a half-finished signals migration. The review closed the accessibility gap,
finished the migration behind an ng update schematic, and added the unit coverage several of its
sub-modules never had.

The second commit fixes a deprecated binding in the docs and a stale invariant comment.

List of notable changes:

  • added the ARIA layer the tree substrate was missing, including the trailing option-action button,
    which gains a locale-backed accessible name (optionActions in KbqA11yLocaleConfiguration)
  • fixed the blur handling in tree-selection.component.ts, so the tree only blurs when the option
    that lost focus is the one the key manager considers active
  • updated the half-finished signals/standalone migration and shipped the tree-signals schematic
    for the renamed surface
  • added unit specs for four sub-modules that had none: flat-tree-control.filters,
    flat-data-source, padding.directive and toggle
  • updated the public API: KBQ_SELECTION_TREE_VALUE_ACCESSOR, KBQ_TREE_OPTION_PARENT_COMPONENT,
    getParents, nodeDefs, the dataSource/indent/recursive accessor pairs and indentUnits all
    changed shape or left the report
  • updated the docs example off the deprecated (onSelectAll) output, which this same branch marks
    @deprecated … will be removed in version 20 in favour of selectAllChange
  • fixed a stale comment in the select-all template: it claimed one @let read covers all four
    reads of selectAllState, which stopped being true when this branch removed two of them; the row's
    own aria-checked host binding is a third, independent read

What should reviewers focus on?

  • packages/schematics/src/migrations/tree-signals/ and its registration.
  • packages/components/core/option/action.ts and core/utils/dom.ts are shared; select and
    tree-select also touch this area in this series.
  • packages/components/tree-select/tree-select.component.ts is modified here as a consumer of the
    changed tree contract, and the tree-select branch modifies the same file.
  • The comment fix is deliberately comment-only: removing the second selectAllTargets walk would need
    a new public input on KbqTreeOption, and the extra walk produces no wrong output.

One of ten pull requests from a single review pass (review/*). Every branch carries two commits: the
fixes from the component review, and a second pass over the findings an independent verifier could not
confirm outright.

The branches share files, so they have to be merged one at a time with a rebase in between. Overlap:
packages/components/core/locales/* (7 branches), tools/public_api_guard/components/core.api.md (7),
tools/cspell-locales/*.json (6), packages/e2e/routes.ts (4) and
packages/schematics/src/{collection,migrations}.json (3).

@github-actions

github-actions Bot commented Aug 26, 2026

Copy link
Copy Markdown

🚨 E2E tests failed

Review the report for details.

💡 Comment /approve-snapshots to approve snapshot changes.

@github-actions github-actions Bot added the bug Something isn't working label Aug 26, 2026
@github-actions

github-actions Bot commented Aug 26, 2026 •
edited
Loading

Copy link
Copy Markdown

Visit the preview URL for this PR (updated for commit 6181f80):

https://koobiq-next--prs-1946-tmsv6fhb.web.app

(expires Sun, 06 Sep 2026 18:44:10 GMT)

🔥 via Firebase Hosting GitHub Action 🌎

Sign: c9e37e518febda70d0317d07e8ceb35ac43c534c

@github-actions

github-actions Bot commented Aug 26, 2026

Copy link
Copy Markdown

🚨 E2E tests failed

Review the report for details.

💡 Comment /approve-snapshots to approve snapshot changes.

@lskramarov lskramarov self-assigned this Aug 27, 2026
@lskramarov
lskramarov force-pushed the review/tree branch 2 times, most recently from 3e663b1 to 88ec30a Compare September 1, 2026 15:50
@github-actions

github-actions Bot commented Sep 1, 2026

Copy link
Copy Markdown

🚨 E2E tests failed

Review the report for details.

💡 Comment /approve-snapshots to approve snapshot changes.

@lskramarov
fix(tree): errors following a full review of the component
b8062bd
@lskramarov
fix(tree): address code review findings
7ea6744
@lskramarov
fix(tree): keep branch rows aligned with their leaf siblings
f2770fa 
Defining --kbq-tree-size-toggle-padding turned on two padding declarations that had
never resolved, widening .kbq-tree-node-toggle from 16px to 24px. The 24px that
KbqTreeNodePadding.iconWidth reserves for a node without a toggle stands in for the
16px toggle plus the option's 8px gap, so every row with a chevron ended up 8px to
the right of its leaf siblings. The contract is now written down next to iconWidth.

Pointing .kbq-option-text at --kbq-tree-size-text-padding-vertical fixed the
reference but changed the value from 0 to 2px, growing every two-line row by 4px and
dropping its text below the chevron and checkbox beside it, whose margins are tuned
to a text box with no padding of its own.
@lskramarov
chore(tree): refresh the api snapshot for the documented iconWidth
46a72f0 
Documenting KbqTreeNodePadding.iconWidth drops its `// (undocumented)` marker from the
report. Content taken verbatim from the diff API Extractor printed on CI.
@lskramarov
docs(tree): document the tree review in the migration guide
be4e6fc 
The tree-signals schematic shipped without a section in the upgrade guide, and this is
the one review whose unmigrated call sites throw rather than fail to compile: assigning
to a getter-only property is a TypeError in any strict-mode build.
@lskramarov
fix(tree): keep the typed selection model main settled on
490057b 
`feat(list,tree,core)!: allow changing multiple after initialization` builds the model as
`SelectionModel<SelectionModelOption>` and swaps the instance when the multiplicity changes,
so the review's widening back to `any` is dropped along with its argument that no call site
produces that shape — and the `any` ratchet prefers the typed one either way.
@lskramarov
chore(tree): refresh the api snapshot after the rebase
5eb141b 
The snapshot came from upstream while the rebase ran, so it carried none of the review's
narrowings. Regenerated from a fresh build rather than edited by hand.
@lskramarov
fix(tree): keep the spec honest about what the selection model holds
9936123 
Two consequences of following main's `SelectionModel<SelectionModelOption>` instead of the
review's widening. The spec reads a selected node back as the consumer's own type, which the
declared shape does not describe, so the call site casts and says why. And the `any` ratchet
had to record the improvement it produced: tree drops from 41 to 40.
@lskramarov
lskramarov marked this pull request as ready for review September 3, 2026 17:49
Copilot AI lite review requested due to automatic review settings September 3, 2026 17:49
Copilot AI reviewed Sep 3, 2026
View reviewed changes

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

KbqTreeBase.switchDataSource clears the outlet without clearing the levels map, which can retain old node objects and grow memory across data source swaps.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

This PR continues the tree component review work by completing the signals migration and accessibility layer, adding a new warn-only ng update migration schematic for newly read-only members, expanding unit coverage across previously-untested tree submodules, and updating public API snapshots and docs/examples accordingly.

Changes:

  • Added/updated accessibility semantics for the tree container and rows (ARIA roles/properties, activedescendant, option action button labelling).
  • Introduced the tree-signals migration (registered for ng update @koobiq/components@20) to report runtime-breaking writes to now read-only/signal-backed members.
  • Improved tree correctness and coverage (dataSource swap handling, trackBy/identity refresh behavior, keyboard navigation/type-ahead, and multiple new unit spec suites).
File summaries
File Description
tools/public_api_guard/components/tree.api.md Updates the tree public API snapshot for signal-backed members, new outputs, and narrowed types.
tools/public_api_guard/components/core.api.md Updates core public API snapshot for new locale key, aria-label input, and new DOM util export.
tools/cspell-locales/ru.json Adds/adjusts Russian dictionary entries used by cspell in the repo.
tools/cspell-locales/en.json Adds an English dictionary entry used by cspell in the repo.
tools/check-public-api-any/baseline.json Updates the baseline for the repo’s public-API any usage ratchet.
packages/schematics/src/migrations/tree-signals/schema.ts Adds TypeScript schema type for the new migration schematic.
packages/schematics/src/migrations/tree-signals/schema.json Adds JSON schema for the new migration schematic options.
packages/schematics/src/migrations/tree-signals/README.md Documents the tree-signals migration scope and manual actions.
packages/schematics/src/migrations/tree-signals/index.ts Implements the warn-only schematic that scans for affected call sites.
packages/schematics/src/migrations/tree-signals/index.spec.ts Adds unit tests validating schematic reporting and non-writing behavior.
packages/schematics/src/migrations/tree-signals/data.ts Defines the match patterns/messages and summary text for reporting.
packages/schematics/src/migrations.json Registers the tree-signals migration for the ng update path.
packages/schematics/src/collection.json Registers tree-signals in the schematics collection.
packages/docs-examples/components/tree/tree-action-button/tree-action-button-example.ts Updates docs example to use selectAllChange instead of deprecated onSelectAll.
packages/components/tree/tree.ts Adds role="tree" to the base tree host.
packages/components/tree/tree.scss Removes theme emission from this entry and documents where it’s emitted.
packages/components/tree/tree.ru.md Updates RU docs with expanded keyboard behavior and a new accessibility/rendering section.
packages/components/tree/tree.en.md Updates EN docs with expanded keyboard behavior and a new accessibility/rendering section.
packages/components/tree/tree-tokens.scss Adjusts/clarifies token usage and adds design rationale comments.
packages/components/tree/tree-selection.component.ts Adds ARIA host bindings, type-ahead, improved blur handling, new outputs, and height util usage.
packages/components/tree/tree-selection.component.spec.ts Adds extensive unit coverage for new behaviors and accessibility (including axe).
packages/components/tree/tree-option.scss Adjusts option text padding behavior in styles.
packages/components/tree/tree-option.component.ts Adds treeitem ARIA semantics, narrows types, changes focus streams to Observables, adds new outputs and refresh behavior.
packages/components/tree/tree-errors.ts Removes an unused/obsolete error helper.
packages/components/tree/tree-base.ts Reworks nodeDef queries to signals, adds view reuse refresh plumbing, and improves dataSource switching logic.
packages/components/tree/toggle.ts Moves recursive/disabled handling to signals/computed, adds filter-driven disabled state, and hides toggle from AT.
packages/components/tree/toggle.spec.ts Adds unit tests for toggle behavior, recursion, filter disabled state, and teardown.
packages/components/tree/toggle.scss Removes invalid token usage and documents toggle sizing/alignment constraints.
packages/components/tree/padding.directive.ts Migrates indent to signal input, reacts to refreshed nodes, and supports non-option tree nodes.
packages/components/tree/padding.directive.spec.ts Adds unit tests for indent units, updates, toggle spacing, and RTL padding.
packages/components/tree/data-source/flat-data-source.ts Adds docs-private markers and fixes documentation naming for the flattener.
packages/components/tree/data-source/flat-data-source.spec.ts Adds unit tests for flattener expansion and flat data source emissions/filtering.
packages/components/tree/control/nested-tree-control.ts Documents synchronous requirements of getChildren due to descendant traversal behavior.
packages/components/tree/control/flat-tree-control.ts Reworks getParents to derive ancestry from flattened order rather than requiring parent references.
packages/components/tree/control/flat-tree-control.spec.ts Adds test coverage for flattened lookups: descendants, parents, filtering, and value lookup.
packages/components/tree/control/flat-tree-control.filters.ts Improves filter stage documentation and clarifies intended behavior.
packages/components/tree/control/flat-tree-control.filters.spec.ts Adds unit tests for filter pipeline stages and select-all row handling.
packages/components/tree-select/tree-select.component.ts Disables tree type-ahead while the select search field is active.
packages/components/core/utils/dom.ts Adds kbqGetElementHeight shared util and exports it publicly.
packages/components/core/option/action.ts Adds ARIA button semantics and locale-backed accessible name for option action buttons.
packages/components/core/locales/types.ts Extends a11y locale configuration with optionActions.
packages/components/core/locales/tk-TM.ts Adds optionActions locale string for tk-TM.
packages/components/core/locales/ru-RU.ts Adds optionActions locale string for ru-RU.
packages/components/core/locales/pt-BR.ts Adds optionActions locale string for pt-BR.
packages/components/core/locales/es-LA.ts Adds optionActions locale string for es-LA.
packages/components/core/locales/en-US.ts Adds optionActions locale string for en-US.
docs/guides/migration.ru.md Documents the tree signals/read-only member breaking changes and the tree-signals reporter.
docs/guides/migration.en.md Documents the tree signals/read-only member breaking changes and the tree-signals reporter.
Review details
  • Files reviewed: 48/48 changed files
  • Comments generated: 1
  • Review effort level: Lite

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread packages/components/tree/tree-base.ts
@lskramarov
fix(tree): drop the level cache when the data source is switched
6181f80 
`switchDataSource` empties the outlet and resets the differ, so the old nodes are never
reported as removals again — and `applyNodeChanges` is the only place that prunes `levels`.
Every node of every past source stayed in the map, which is keyed by the node objects
themselves, so the tree pinned them for as long as it lived.

Measured on the existing swap fixture: 5 entries before the swap, 10 after, and still 10
after the source is set to null. With main's `switchDataSource` it stays 5 throughout,
because the differ it keeps reports the old nodes as removals.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

Copilot code review Copilot
Copilot review effort, defaults to Lite
Applies to this pull request for everyone.Learn more about Copilot code review.
Copilot left review comments
@artembelik artembelik Awaiting requested review from artembelik
@NikGurev NikGurev Awaiting requested review from NikGurev

At least 1 approving review is required to merge this pull request.

Assignees

@lskramarov lskramarov

Labels

bug Something isn't working

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@lskramarov